fix: store and return TableClass, SSESpecification, OnDemandThroughput#155
Conversation
LeeroyHannigan
requested review from
amrith,
c33howard,
jcshepherd,
pdf-amzn and
yesyayen
as code owners
|
|
||
| #[test] | ||
| fn sse_description_serializes_with_kms_arn() { | ||
| let sse = SseDescription { |
There was a problem hiding this comment.
Does SSEDescription include InaccessibleEncryptionDateTime ? Should it? (Doesn't necessarily have to be part of this review, but it looks to be missing.)
There was a problem hiding this comment.
Not needed as we don't simulate inaccessible keys. The field would always be null. Can add later if a customer scenario requires it.
There was a problem hiding this comment.
Hmm. Okay. It's part of the DDB public API model though, isn't it? I'm okay with taking care of it separately, but it's not clear to me that it can be left out from an API model conformance perspective. (Unless we are accepting it in the API but just ignoring it?)
There was a problem hiding this comment.
Just ignoring it, yes. Its completely pass through. We have no way to simulate a non-conformant KMS key. Because we don't do KMS encryption.
| status: "ENABLED".to_string(), | ||
| sse_type: Some(SseType::KMS), | ||
| kms_master_key_arn: Some(format!( | ||
| "arn:aws:kms:us-east-1:{}:key/default", |
There was a problem hiding this comment.
Is region relevant here at all? Not for ExtendDB functionally, but would customers be surprised if they've configured to run in a different region?
There was a problem hiding this comment.
Definitely, nice catch. Forgot to remove this after changing it for testing.
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
Overall looks okay: couple minor questions.
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
One more question about the SSE thingie, but shouldn't block this.
2 participants
What
Store and return
TableClass,SSESpecification, andOnDemandThroughputfields through CreateTable, DescribeTable, and UpdateTable.TableClasspersists as text and is returned inTableClassSummaryon describeSSESpecification: { Enabled: true }returnsSSEDescriptionwithStatus: ENABLED,SSEType: KMS, and a synthesizedKMSMasterKeyArnOnDemandThroughputpersistsMaxReadRequestUnits/MaxWriteRequestUnitsand round-trips through create/update/describeOnDemandThroughputstruct to core types andKMSMasterKeyArnfield toSseDescriptionThese are passthrough fields only, no behavioral enforcement (no throttling, no encryption, no storage tiering).
Why
Applications that set these fields (CDK, Terraform, SDK calls) should not error out, and tools that read them back (drift detection, state reconciliation) should get what they expect.
Testing done
cargo fmt --all -- --check— cleancargo clippy --all-targets -- -D warnings— cleancargo test --workspace— all pass (5 new unit tests for serde round-trips)tests/test_config_fields.py— 6 integration tests covering create, describe, and update for all 3 fields (all pass against ExtendDB)Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC: n/a
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.